AGPHVT-82 : Improve buffer transitions and content copying between frame passes. #85

AdamFelt · 2025-10-14T19:20:29Z

This submission fixes render buffer handling between renderers and frame passes.

Removes the CopyTask and moves copy functionality to the RenderBufferManager.
Moves AOVInput and RenderTask gathering out of the update function and puts it in the render loop.
Texture output contents are now copied to the next pass buffer inputs between passes, not as part of the pass.
RenderBuffers are reused from the previous pass not the first pass which supports more flexible mixing of renderers.
New render buffers are created if the render switches between passes so renderers are no longer rendering into a render buffer they did not create. RenderBuffers are still reused from previous passes if the renderer does not change.

Signed-off-by: Patrick Hodoul <[email protected]>

…Transition

…ng the right thing.

# Conflicts: # source/engine/renderBufferManager.cpp # test/tests/testFramePasses.cpp

…Transition

sebastienberube-adsk

I left a bunch of observation comments to understand and document the PR.
You can ignore most of them, but maybe these points stand out a little more:

Use of TkToken instead of string for RenderBufferBinding::rendererName
Removal of CreateRenderBufferProxy class in framePassUtils.h/cpp
- I believe the class isn't relevant anymore, since the RenderBufferManager now supports scenarios with mixed render delegates. As far as I know, the CreateRenderBufferProxy class was only added to support these scenarios, which is now redundant.
Add check or assert to make sure the colorInput and colorDesc are for a "color" aov
(I believe there is an implicit assumption the color is always first, but just in case, I would add something to double-check- and protect this assumption against future changes.
See in RenderBufferManager.cpp (ln :558-562)

                else if (!colorInput.texture)
                {
                    colorDesc  = desc;
                    colorInput = input;
                }

sebastienberube-adsk · 2025-10-20T15:29:28Z

source/engine/framePass.cpp

-                HdAovTokens->instanceId },
-            {},
-            _passParams.renderParams.viewport); // NOTE: this is the non-adjusted viewport.
-


I never understood the exact reason why initializing AOV buffers was necessary when using a renderer other than Storm.

I can see similar code being executed with the TaskController, from the constructor:
HdxTaskController::HdxTaskController -> HdxTaskController::_CreateRenderGraph() -> HdxTaskController::SetRenderOutputs("color")

Since I do not understand why this line had been added to the TaskController, I am slightly worried removing it here might break something.

However, on the other hand, I can see that _bufferManager->SetRenderOutputs is called early, in the beggining of FramePass::GetRenderTasks. Furthermore, all task update code (the commit functions) are called at the very end of FramePass::GetRenderTasks, which means not much is happening before SetRenderOutputs is called. So maybe it does not change anything to make this call later, given the order of things with the FramePass.

In conclusion, removing _bufferManager->SetRenderOutputs from this task creation function in the FramePass is probably safe, and this call might only be needed with the TaskController because it has a different task parameter update sequence, requiring the early call to HdxTaskController::SetRenderOutputs("color").

See:
https://git.autodesk.com/autodesk-forks/USD/blob/d34c093b8126e5b82fa35b478586539f82ceadd7/pxr/imaging/hdx/taskController.cpp#L292C13-L292C29

sebastienberube-adsk · 2025-10-20T15:35:40Z

source/engine/framePass.cpp

+hvt::RenderBufferBindings FramePass::GetRenderBufferBindingsForNextPass(
+    std::vector<pxr::TfToken> const aovs, bool copyContents)
+{
+    std::string renderName = GetRenderIndex()->GetRenderDelegate()->GetRendererDisplayName();


Suggested change

std::string renderName = GetRenderIndex()->GetRenderDelegate()->GetRendererDisplayName();

std::string const& renderName = GetRenderIndex()->GetRenderDelegate()->GetRendererDisplayName();

Actually, a TfToken would be preferable here I think. The TfToken for the rendererName could be stored in the FramePass of the RenderBufferManager in FramePass::Initialize.

sebastienberube-adsk · 2025-10-20T18:44:44Z

source/engine/renderBufferManager.cpp

+    HdAovDescriptorList outputDescs;

    // NOTE: A function could be used to get localOutputs.
    TfTokenVector localOutputs = outputs;


[Observation]

This is out of scope for this PR since the name did not change, but we could rename localOutputs to something more descriptive of what it actually is, something like supportedOutputs maybe, since the only difference that I see between outputs and localOutputs is that each entry is validated as shown below:

for (auto it = localOutputs.begin(); it != localOutputs.end();) { HdAovDescriptor desc = _pRenderIndex->GetRenderDelegate()->GetDefaultAovDescriptor(*it); if (desc.format == HdFormatInvalid) { // The backend doesn't support this AOV, so skip it. it = localOutputs.erase(it); }

I think that "outputs" is a very confusing name, as indeed it is an input to the render pass. To me, "local" is ok, or maybe better yet "temp", because they are what is being computed in this function. "Supported" is ok too and is actually what it is filtering on. I don't want to change this now though because this PR is already too big.

sebastienberube-adsk · 2025-10-20T18:50:22Z

source/engine/renderBufferManager.cpp


-    // If progressive rendering is enabled, render buffer clear is only required when `_aovOutputs
-    // != outputs`.
-    bool needClear = !_isProgressiveRenderingEnabled || _aovOutputs != outputs;


[Observation]

bool needClear = !_isProgressiveRenderingEnabled || _aovOutputs != outputs;

I see this was moved below and is only executed if (somethingChanged) which should be functionally the same as before, with the early return if cached inputs and outputs have changed.

Yeah, we just can't bail out early because we need to do the copy, but the functionality should still be skipped.

sebastienberube-adsk · 2025-10-20T18:52:15Z

source/engine/renderBufferManager.cpp

-    {
-        std::copy(inputs.begin(), inputs.end(), back_inserter(_aovInputs));
-        _viewportAov = TfToken();
-    }


I see this block was also moved below and is only executed if (somethingChanged), which makes sense and should deliver the same outcome as before:

_aovInputs.clear(); if (inputs.size() > 0) { std::copy(inputs.begin(), inputs.end(), back_inserter(_aovInputs)); _viewportAov = TfToken(); }

sebastienberube-adsk · 2025-10-20T19:27:27Z

source/engine/renderBufferManager.cpp

    }
+    if (colorInput.texture)
+    {
+        PrepareBuffersFromInputs(colorInput, depthInput, colorDesc, controllerId);


[Observation]

My understanding is this function replaces the CopyTask.

sebastienberube-adsk · 2025-10-20T19:32:36Z

source/engine/renderBufferManager.cpp

+        // Note, it would be better to just assign the output buffer here, but this breaks some
+        // unit tests that expect this to be null and do a pointer-as-string comparison if it is not 
+        // which is not easily fixable.


[Question]

Which test is this breaking?
(If the test is TestTaskParameters, I could probaly do something about it)

In any case, this appears like a minor thing, won't block the PR for this, I am mostly curious of the test this would break.

There are two tests that compare the task parameters. They just use the io << functionality so the pointer is written out deep in Hydra.

sebastienberube-adsk · 2025-10-20T19:35:08Z

source/engine/renderBufferManager.cpp

-        HdRenderBuffer* firstInput = nullptr;
-        HdRenderBuffer* depthInput = nullptr;
-        HdRenderBuffer* neyeInput  = nullptr;
-        for (auto input : inputs)
-        {
-            // This is super fragile and limited.
-            // We should be able to pass more inputs to other passes.
-            if (input.first == localOutputs[0])
-                firstInput = input.second;
-            if (input.first == "depth")
-                depthInput = input.second;
-            if (input.first == HdAovTokens->Neye)
-                neyeInput = input.second;


[Observation]

I am guessing this part, where the 3 inputs are collected before calling SetViewportRenderOutput, is not longer relevant since the function calls GetRenderOutput(color/depth/Neye) internally to get each one of them.

Yeah, this is no longer needed.

sebastienberube-adsk · 2025-10-20T19:36:45Z

source/tasks/copyTask.cpp

Glad this was removed. Less complexity.

sebastienberube-adsk · 2025-10-20T19:55:11Z

include/hvt/engine/renderBufferSettingsProvider.h

+    PXR_NS::TfToken aovName;
+    PXR_NS::HgiTextureHandle texture;
+    PXR_NS::HdRenderBuffer* buffer;
+    std::string rendererName;


Suggestion: use a TfToken for the renderName instead of a string.

Since TfToken creation is somewhat expensive, it could be created once in FramePass::Initialize(), then stored as a FramePass or RenderBufferManager member variable and assigned in the RenderBufferBinding without having to deal with the string representation after FramePass::Initialize().

Comparisons and assignments of TfTokens are cheap, once converted from a string.

Yeah, could be. This is not executed in a deep or large loop so there will be no real performance improvement. I wish the name from Hydra was a token to begin with, or there was a way to get the more accurate name "HdStormRenderDelegate". We could store it in the frame pass, but I would like to make this change in a second PR not this one.

hodoulp · 2025-10-21T12:25:30Z

include/hvt/engine/framePass.h

    /// Returns the default selection accessor.
    SelectionSettingsProviderWeakPtr GetSelectionSettingsAccessor() const;

+    // Returns true if the frame pass should be rendered.


Be careful Doxygen comments are /// (and not //.

I'll fix this.

hodoulp · 2025-10-21T12:26:27Z

include/hvt/engine/renderBufferSettingsProvider.h

+{
+    PXR_NS::TfToken aovName;
+    PXR_NS::HgiTextureHandle texture;
+    PXR_NS::HdRenderBuffer* buffer;


PXR_NS::HdRenderBuffer* buffer; -> PXR_NS::HdRenderBuffer* buffer = nullptr;

Yes. I will fix.

hodoulp · 2025-10-21T12:35:58Z

source/engine/renderBufferManager.cpp

+            if (input.aovName == localOutputs[i])
            {
-                foundInput = input.second;
+                inputFound = (rendererName == input.rendererName);


Is the name comparison enough?

The difference is not on the render delegate name but rather on CPU vs. GPU and texture formats. For example, if two different render delegates are both GPU & same texture formats, the code should not add extra copy (ies). I assume that it will be the case between Flash and Storm for example.

hodoulp · 2025-10-21T12:40:05Z

source/tasks/aovInputTask.cpp

    ctx->erase(HdAovTokens->depth);
    ctx->erase(HdxAovTokens->colorIntermediate);
    ctx->erase(HdAovTokens->Neye);
-    ctx->erase(msaaToken);


@sebastienberube-adsk It seems that the neye rework remains. That could be a in Q4 dedicated backlog as discussed.

Yeah, I didn't touch Neye. It still needs to be fixed. It should not be on the context. The context is for things that are read and rewritten, not just read.

hodoulp · 2025-10-21T12:42:14Z

source/engine/framePass.cpp

 }

+hvt::RenderBufferBindings FramePass::GetRenderBufferBindingsForNextPass(
+    std::vector<pxr::TfToken> const aovs, bool copyContents)


std::vector<pxr::TfToken> const aovs -> std::vector<pxr::TfToken> const& aovs

…Transition

hodoulp and others added 19 commits September 17, 2025 16:01

OGSMOD-8014 - Fix cmake module issue

134d34a

Signed-off-by: Patrick Hodoul <[email protected]>

Merge branch 'main' into hodoulp/cmake_module

5b52ab7

Provide built in support for copying output between passes.

6a1610c

Merge remote-tracking branch 'origin/main' into felta/Improve-Buffer-…

9b1c94c

…Transition

Add an enabled property to frame pass.

207dccf

Fix unit tests

c5e494d

Remove InputAOV and Task reporting from FramePass::Update()

3b4769b

Remove the CopyTask as it is no longer needed

114e579

Merge remote-tracking branch 'origin/main' into felta/Improve-Buffer-…

a07bd8c

…Transition

Temporarily modify a test because it is not working properly or testi…

6a5fd3f

…ng the right thing.

Incorperate a fix found by Sebastien in this branch

dd96c7c

Fix for Embree errors

54584c3

Add robustness and sanity checking when getting the Hgi instance.

ffc155e

Merge branch 'main' into felta/Improve-Buffer-Transition

1a8dfeb

# Conflicts: # source/engine/renderBufferManager.cpp # test/tests/testFramePasses.cpp

Fix merge error

cc61ac8

We can just use the default HdxFullScreenShader

0268550

Improve function comments.

b2c2f74

Remove unused code.

f58d4b0

Not my change.

049f4a8

AdamFelt requested a review from sebastienberube-adsk October 14, 2025 20:03

AdamFelt added 6 commits October 15, 2025 12:33

Fix build errors and more validity checking

44974b5

Protect against Embree still rendering.

ecd489b

Trying to fix build error.

3c81144

Another attempt at a fix.

4d3a12a

Add WaitForGPUIdle to several tests that were missing it.

374ae01

Merge remote-tracking branch 'origin/main' into felta/Improve-Buffer-…

0288104

…Transition

sebastienberube-adsk approved these changes Oct 20, 2025

View reviewed changes

hodoulp reviewed Oct 21, 2025

View reviewed changes

AdamFelt added 2 commits October 28, 2025 07:41

Code review feedback improvements

e991466

Merge remote-tracking branch 'origin/main' into felta/Improve-Buffer-…

20363c3

…Transition

AdamFelt merged commit 1dd9b71 into main Oct 28, 2025
10 checks passed

AdamFelt deleted the felta/Improve-Buffer-Transition branch October 28, 2025 18:54

	std::string renderName = GetRenderIndex()->GetRenderDelegate()->GetRendererDisplayName();
	std::string const& renderName = GetRenderIndex()->GetRenderDelegate()->GetRendererDisplayName();

AGPHVT-82 : Improve buffer transitions and content copying between frame passes. #85

AGPHVT-82 : Improve buffer transitions and content copying between frame passes. #85

Uh oh!

Conversation

AdamFelt commented Oct 14, 2025

Uh oh!

sebastienberube-adsk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienberube-adsk Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodoulp Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodoulp Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebastienberube-adsk left a comment •

edited

Loading

sebastienberube-adsk Oct 20, 2025 •

edited

Loading

hodoulp Oct 21, 2025 •

edited

Loading

hodoulp Oct 21, 2025 •

edited

Loading